Conversation
allenporter
left a comment
There was a problem hiding this comment.
I am very happy to keep momentum going on this. I am adding additional feedback since this hasn't been moving since my last comment, in case it was missed. I have a lot of time to review during the us holidy weekend.
|
@allenporter Thanks for the detailed feedback, and sorry I missed the earlier comments. Pushed cbd6abc and merged current main.
Validation: 999 tests and 92 snapshots passed, all pre-commit checks passed, and wheel/sdist builds passed. The dependent read-only PRs have been updated as well. Could you take another look when you have time, particularly at the shared-map/dedicated-clean-record type boundary? These maintenance changes were prepared with OpenAI Codex assistance. |
allenporter
left a comment
There was a problem hiding this comment.
Thanks for iterating on this and splitting out the clean-record path ownership.
Taking a close look at the resulting Q10CleanRecordMapPacket boundary, the **vars(packet) unpacking in parse_map_packet and the isinstance check in render_q10_map feel like an awkward fit for dataclass inheritance. Rather than subclassing Q10MapPacket, modeling this as a composite object Q10CleanRecordDetail(map=Q10MapPacket, trace=Q10HistoricalTracePacket | None) would be significantly cleaner and avoid both constructor duplication and isinstance branches in rendering.
I've also left a few inline comments on RoborockEnum fallback resilience, stripping stray origin points, and trimming internal wire fields from public models.
|
@allenporter Implemented the requested composition in 4eb0b9b and updated the consumers in #937–#939:
Merged current main and preserved the new position/zone/goto support. Full local validation on #936: 1349 passed, 53 existing xfailed, 92 snapshots passed; pre-commit and package builds passed. Separately, current main's new model conformance test fails on the existing frozen Could you review the revised boundary and parser changes when you have time? |
|
@hCoureau @allenporter This PR is really valuable for Q10 map support, but it's been waiting on the Sep 12 architecture feedback. Do you want help picking up the composition refactor? The changes are clear:
@allenporter, should hCoureau tackle these all at once, or is there a staged approach you'd prefer? Happy to pair on implementation or review if that unblocks things. |
|
Thanks — I’ve already implemented the full set of Sep 12 changes in 4eb0b9b, including the Q10CleanRecordDetail(map, trace) composition model, enum fallback handling, removal of unused helpers, internal-field cleanup, and the historical-trace filtering. I also updated the dependent consumers in #937–#939 and resolved the inline threads. |
Q10 clean-record and saved-map responses use different packet markers from live maps. This adds bounded archive parsing and a
Q10CleanRecordDetail(map, trace)response so callers receive the map and historical path together without extending the map raster model.Implementation
Q10MapPacketKind. Unknown values resolve tounknownand are ignored rather than defaulting to a current map.Q10HistoricalTracePacketlimited to points and heading. Validate wire version/reserved fields internally and do not expose the undecoded wire value.#937 consumes the new detail object in its clean-history trait. Saved and live map state remain independent. No private map captures or account data are included.
Dependencies and validation
#933 is merged. Current upstream
main(eac001c, including #908) is integrated. This branch also includes the isolated conformance fix in #965, which addresses the independently reproduced upstream failure tracked in #964; please review/merge that small fix first.Related: #767.